Skip to content

[dm][rpmsg] support Remote Processor Messaging (RPMSG)#11303

Open
GuEe-GUI wants to merge 1 commit intoRT-Thread:masterfrom
GuEe-GUI:dm_rpmsg
Open

[dm][rpmsg] support Remote Processor Messaging (RPMSG)#11303
GuEe-GUI wants to merge 1 commit intoRT-Thread:masterfrom
GuEe-GUI:dm_rpmsg

Conversation

@GuEe-GUI
Copy link
Copy Markdown
Contributor

@GuEe-GUI GuEe-GUI commented Mar 29, 2026

拉取/合并请求描述:(PR description)

[
support Remote Processor Messaging, The virtio driver will support in QEMU

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/ALL_BSP_COMPILE.json 详细请参考链接BSP自查

Signed-off-by: GuEe-GUI <2991707448@qq.com>
@github-actions
Copy link
Copy Markdown

📌 Code Review Assignment

🏷️ Tag: components

Reviewers: @Maihuanyi

Changed Files (Click to expand)
  • components/drivers/Kconfig
  • components/drivers/include/drivers/rpmsg.h
  • components/drivers/include/rtdevice.h
  • components/drivers/rpmsg/Kconfig
  • components/drivers/rpmsg/SConscript
  • components/drivers/rpmsg/rpmsg-rt-thread-virtio.c
  • components/drivers/rpmsg/rpmsg.c
  • components/drivers/rpmsg/rpmsg_char.c
  • components/drivers/rpmsg/rpmsg_ns.c

📊 Current Review Status (Last Updated: 2026-03-29 22:30 CST)


📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

@github-actions
Copy link
Copy Markdown

👋 感谢您对 RT-Thread 的贡献!Thank you for your contribution to RT-Thread!

为确保代码符合 RT-Thread 的编码规范,请在你的仓库中执行以下步骤运行代码格式化工作流(如果格式化CI运行失败)。
To ensure your code complies with RT-Thread's coding style, please run the code formatting workflow by following the steps below (If the formatting of CI fails to run).


🛠 操作步骤 | Steps

  1. 前往 Actions 页面 | Go to the Actions page
    点击进入工作流 → | Click to open workflow →

  2. 点击 Run workflow | Click Run workflow

  • 设置需排除的文件/目录(目录请以"/"结尾)
    Set files/directories to exclude (directories should end with "/")
  • 将目标分支设置为 \ Set the target branch to:dm_rpmsg
  • 设置PR number为 \ Set the PR number to:11303
  1. 等待工作流完成 | Wait for the workflow to complete
    格式化后的代码将自动推送至你的分支。
    The formatted code will be automatically pushed to your branch.

完成后,提交将自动更新至 dm_rpmsg 分支,关联的 Pull Request 也会同步更新。
Once completed, commits will be pushed to the dm_rpmsg branch automatically, and the related Pull Request will be updated.

如有问题欢迎联系我们,再次感谢您的贡献!💐
If you have any questions, feel free to reach out. Thanks again for your contribution!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces an RPMsg (Remote Processor Messaging) subsystem under RT-Thread’s driver model, aiming to provide endpoint management, name-service handling, and a character-device interface, with an intended VirtIO-based transport for QEMU.

Changes:

  • Add RPMsg core bus/device/endpoint APIs (rpmsg.c) and public header (drivers/rpmsg.h).
  • Add RPMsg name-service driver (rpmsg_ns.c) and RPMsg char/control device (rpmsg_char.c).
  • Add build integration via components/drivers/rpmsg/{Kconfig,SConscript} and hook into components/drivers/Kconfig + rtdevice.h.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
components/drivers/rpmsg/rpmsg.c RPMsg bus registration and endpoint lifecycle + send APIs
components/drivers/rpmsg/rpmsg_char.c RPMsg char/control device implementation with endpoint create/destroy
components/drivers/rpmsg/rpmsg_ns.c RPMsg name-service message handling and driver registration
components/drivers/rpmsg/rpmsg-rt-thread-virtio.c Intended VirtIO transport glue for RPMsg over mailbox/shared-memory
components/drivers/rpmsg/SConscript Build group and source selection for RPMsg module
components/drivers/rpmsg/Kconfig Kconfig options for enabling RPMsg + char limits + virtio option
components/drivers/include/drivers/rpmsg.h Public RPMsg API/types exported to the system
components/drivers/include/rtdevice.h Expose RPMsg header via rtdevice.h when enabled
components/drivers/Kconfig Adds rpmsg Kconfig entry to drivers menu

Comment on lines +273 to +275
rt_device_register(&rch->parent, rt_dm_dev_get_name(&rch->parent),
RT_DEVICE_FLAG_RDWR | RT_DEVICE_FLAG_REMOVABLE);

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: rt_device_register() return value is ignored. If registration fails, the code still returns RT_EOK, leaking the endpoint/device-id and leaving an unregistered device object in the list.

English: Check the return value of rt_device_register() and on failure roll back: remove from list, free IDA id, destroy endpoint, and free rch.
中文:这里忽略了 rt_device_register() 的返回值;如果注册失败,代码仍然返回 RT_EOK,会造成 endpoint/device-id 泄漏,并且链表中残留未注册的设备对象。请检查 rt_device_register() 的返回值,失败时回滚:从链表移除、释放 IDA、销毁 endpoint,并释放 rch

Copilot uses AI. Check for mistakes.
Comment on lines +373 to +376
rt_device_unregister(&rchc->parent);

rt_free(rchc);

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[concurrency/并发问题]: del_ept_work can be (re)submitted asynchronously, but rpmsg_char_remove() frees rchc without canceling the pending work item. This can lead to the workqueue executing with freed work_data (use-after-free).

English: Call rt_work_cancel(&rchc->del_ept_work) (or a sync variant if needed) before freeing rchc, and ensure no further resubmits occur during teardown.
中文:del_ept_work 可能被异步提交/重复提交,但 rpmsg_char_remove() 在未取消该 work 的情况下就释放了 rchc,可能导致 workqueue 在 work_data 已释放时执行(UAF)。建议在释放 rchc 前调用 rt_work_cancel(&rchc->del_ept_work)(必要时用同步取消接口),并确保卸载过程中不会再次提交。

Copilot uses AI. Check for mistakes.
struct rt_rpmsg_endpoint *ept;
struct rt_rpmsg_endpoint_info info;

RT_ASSERT(len <= RT_RPMSG_CHAR_MSG_SIZE_MAX);
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[operational_implications/运行风险]: RX length is validated with RT_ASSERT(). Since RPMsg payload comes from a remote processor, a malformed/oversized message could trigger an assert and crash the system.

English: Prefer a runtime check (e.g., if (len > RT_RPMSG_CHAR_MSG_SIZE_MAX) return -RT_EINVAL;) and drop/log the frame instead of asserting.
中文:这里用 RT_ASSERT() 校验接收长度。RPMsg 数据来自对端处理器,若收到畸形/超长消息可能触发断言导致系统崩溃。建议改为运行时判断(例如 len > RT_RPMSG_CHAR_MSG_SIZE_MAX 则返回错误并丢包/记录日志),避免断言。

Suggested change
RT_ASSERT(len <= RT_RPMSG_CHAR_MSG_SIZE_MAX);
if (len > RT_RPMSG_CHAR_MSG_SIZE_MAX)
{
LOG_W("rx msg too long: len=%d, max=%d", (int)len, RT_RPMSG_CHAR_MSG_SIZE_MAX);
return -RT_EINVAL;
}

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +22
#include "../virtio/virtio_config/virtio-rpmsg.h"
#include "../virtio/virtio_internal.h"

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: The included headers appear to not exist in this repository (../virtio/virtio_config/virtio-rpmsg.h, ../virtio/virtio_internal.h), and the file also uses struct rt_virtio_device/rt_virtqueue_* APIs that cannot be found in-tree.

English: Either add the missing virtio headers/APIs in this PR, or refactor this driver to use the existing components/drivers/virtio/* interfaces (struct virtio_device, virtq, etc.) so the build succeeds.
中文:当前包含的头文件在仓库内似乎不存在(../virtio/virtio_config/virtio-rpmsg.h../virtio/virtio_internal.h),且代码使用了仓库中查不到的 struct rt_virtio_device/rt_virtqueue_* 接口,会导致无法编译。请在本 PR 补齐缺失的 virtio 头文件/接口,或改为基于现有 components/drivers/virtio/* 的接口实现(struct virtio_devicevirtq 等)。

Copilot uses AI. Check for mistakes.
rv->is_master = rt_dm_dev_prop_read_bool(dev, "master");
rv->regs = rv->shmem;
rv->pool = rv->shmem + ARCH_PAGE_SIZE;

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[operational_implications/运行风险]: size = (size - ARCH_PAGE_SIZE) >> 1; will underflow if the shared-memory region is smaller than (or equal to) ARCH_PAGE_SIZE, producing a huge size and causing out-of-bounds pool setup.

English: Add a sanity check that size > ARCH_PAGE_SIZE (and that the remaining size is large enough for two halves) before subtracting/shifting.
中文:size = (size - ARCH_PAGE_SIZE) >> 1; 在共享内存区域大小小于等于 ARCH_PAGE_SIZE 时会发生下溢,导致 size 变成很大的值,从而出现越界的 pool 计算。建议在减法/移位前检查 size > ARCH_PAGE_SIZE,并确认剩余空间足够分成两半。

Suggested change
if (size <= ARCH_PAGE_SIZE)
{
LOG_E("Invalid shmem size (0x%lx), must be larger than 0x%lx",
(unsigned long)size, (unsigned long)ARCH_PAGE_SIZE);
err = -RT_EINVAL;
goto _fail;
}

Copilot uses AI. Check for mistakes.
Comment on lines +179 to +184
rt_uint32_t event;
struct rpmsg_virtio *rv = raw_to_rpmsg_virtio(vq->vdev);

event = RPMSG_VIRTIO_EVENT_QUEUE;
rt_mbox_send(rv->chan, &event, RT_WAITING_FOREVER);

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: Use-after-free risk: rt_mbox_send() stores the data pointer in chan->data until rt_mbox_send_done() runs, so passing the address of a stack variable (&event) is unsafe and can be dereferenced after the function returns.

English: Use a persistent buffer for the mailbox payload (e.g., store event in struct rpmsg_virtio or allocate it) and pass that pointer to rt_mbox_send().
中文:rt_mbox_send() 会把 data 指针保存到 chan->data 直到发送完成回调执行,因此这里把栈变量地址(&event)传进去会在函数返回后变成悬空指针。请使用持久化的缓冲区(例如把 event 放到 struct rpmsg_virtio 里或动态分配)再传给 rt_mbox_send()

Copilot uses AI. Check for mistakes.
Comment on lines +305 to +308
/* Ask peer to link */
event = RPMSG_VIRTIO_EVENT_LINK;
rt_mbox_send(rv->chan, &event, RT_WAITING_FOREVER);

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: Same stack-lifetime issue as above: &event is a stack pointer passed to rt_mbox_send() and may be used after return.

English: Make the mailbox payload storage persistent (e.g., rv->event_link) before calling rt_mbox_send().
中文:同样的栈生命周期问题:把 &event(栈地址)传给 rt_mbox_send() 可能在函数返回后被继续使用。请改为使用持久化存储(例如 rv->event_link)后再调用 rt_mbox_send()

Copilot uses AI. Check for mistakes.
Comment on lines +475 to +476
rt_ofw_get_address(shmem_np, 0, &addr, &size);
rt_ofw_node_put(shmem_np);
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: rt_ofw_parse_phandle() can return NULL; rt_ofw_get_address() also returns an error code. Here neither is checked before use, which can lead to a NULL dereference or using uninitialized addr/size.

English: Validate shmem_np != RT_NULL and check the return value of rt_ofw_get_address() (and handle failures with cleanup) before continuing.
中文:rt_ofw_parse_phandle() 可能返回 NULL,rt_ofw_get_address() 也会返回错误码;这里未做检查就直接使用,会导致空指针解引用或使用未初始化的 addr/size。请先判断 shmem_np != RT_NULL 并检查 rt_ofw_get_address() 的返回值,失败时走清理流程。

Suggested change
rt_ofw_get_address(shmem_np, 0, &addr, &size);
rt_ofw_node_put(shmem_np);
if (shmem_np == RT_NULL)
{
err = -RT_EINVAL;
LOG_E("Get shmem node fail");
goto _fail;
}
err = rt_ofw_get_address(shmem_np, 0, &addr, &size);
rt_ofw_node_put(shmem_np);
if (err)
{
LOG_E("Get shmem address fail");
goto _fail;
}

Copilot uses AI. Check for mistakes.
if (err && err != -RT_EEMPTY)
{
LOG_E("Attach power domain error = %s in device %s",
rt_dm_dev_get_name(dev), rt_strerror(err));
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: Log arguments are swapped: format is "Attach power domain error = %s in device %s" but it passes (device_name, strerror).

English: Swap the arguments so the first %s is rt_strerror(err) and the second is the device name.
中文:日志参数顺序写反了:格式为 "Attach power domain error = %s in device %s",但实际传参是 (device_name, strerror)。请交换参数,使第一个 %s 对应 rt_strerror(err),第二个 %s 对应设备名。

Suggested change
rt_dm_dev_get_name(dev), rt_strerror(err));
rt_strerror(err), rt_dm_dev_get_name(dev));

Copilot uses AI. Check for mistakes.
}

if ((device_id = rt_dm_ida_alloc(&rpmsg_char_ida)) < 0)
{
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug/缺陷]: Memory leak on rt_dm_ida_alloc() failure: rchc is allocated but not freed when rt_dm_ida_alloc(&rpmsg_char_ida) fails.

English: Free rchc (and ensure rdev->parent.user_data isn’t left pointing to freed/partial state) before returning.
中文:rt_dm_ida_alloc() 失败时会内存泄漏:rchc 已经分配,但在 rt_dm_ida_alloc(&rpmsg_char_ida) 失败后直接返回,没有释放。请在返回前释放 rchc(并确保 rdev->parent.user_data 不会指向半初始化状态)。

Suggested change
{
{
rt_free(rchc);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants